Skip to content

Process cleanup #3877

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 5 commits into from
Jun 16, 2024
Merged

Process cleanup #3877

merged 5 commits into from
Jun 16, 2024

Conversation

@djc
Copy link
Contributor

@djc djc commented Jun 16, 2024

This is from last night. I think the custom test macros are unnecessary now, please review!

@djc djc requested a review from rami3l June 16, 2024 06:16
@djc djc force-pushed the process-cleanup branch from 73f9225 to 1dee499 Compare June 16, 2024 06:17
rami3l
rami3l reviewed Jun 16, 2024
View reviewed changes
rami3l
rami3l approved these changes Jun 16, 2024
View reviewed changes
Copy link
Member

@rami3l rami3l left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow that's soooo impressive, friend!

Rustup is finally an "ordinary" tokio app now thanks to your work! Hooray 🎉

@rami3l rami3l mentioned this pull request Jun 16, 2024
Open
@djc djc force-pushed the process-cleanup branch from 1dee499 to 0796d14 Compare June 16, 2024 08:30
@djc
Copy link
Contributor Author

djc commented Jun 16, 2024

Rustup is finally an "ordinary" tokio app now thanks to your work! Hooray 🎉

Our work, couldn't have done it without you!

@djc djc force-pushed the process-cleanup branch from 0796d14 to 4ef3559 Compare June 16, 2024 08:48
@djc djc enabled auto-merge June 16, 2024 08:49
@rami3l rami3l added this to the 1.28.0 milestone Jun 16, 2024
@djc djc added this pull request to the merge queue Jun 16, 2024
@rami3l
Copy link
Member

rami3l commented Jun 16, 2024 •
edited
Loading

@djc Let's don't forget there's one remaining concern from #3847 (comment): there are places where the original code was using #[::tokio::test(flavor = "multi_thread", worker_threads = 1)] (you just deleted these lines in this PR), while worker_threads = 2 was used in with_runtime() instead:

rustup/src/test/mock/clitools.rs

Lines 729 to 738 in 0aa7a2a

let mut builder = Builder::new_multi_thread();
builder
.enable_all()
.worker_threads(2)
.max_blocking_threads(2);
let process_res = currentprocess::with_runtime(
tp.clone().into(),
builder,
rustup_mode::main(tp.cwd.clone()),
);

I'm not quite sure about the exact intention of those configurations, but it's better to be watchful I think.

Merged via the queue into master with commit a10ad73 Jun 16, 2024
@djc djc deleted the process-cleanup branch June 16, 2024 09:32
This was referenced Jul 30, 2024
Closed
Merged
@rami3l rami3l mentioned this pull request Oct 6, 2024
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rami3l rami3l rami3l approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

1.28.0

Development

Successfully merging this pull request may close these issues.

3 participants

@djc @rami3l